Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Feat: Enable hosting app in any route, with easy config #3722

Merged
merged 2 commits into from
Nov 29, 2023
Merged

Feat: Enable hosting app in any route, with easy config #3722

merged 2 commits into from
Nov 29, 2023

Conversation

haricane8133
Copy link
Contributor

Pull Request type

  • Bugfix
  • Feature
  • Refactoring (no functional changes, no api changes)
  • Build related changes (Please run ./gradlew generateLock saveLock to refresh dependencies)
  • WHOSUSING.md
  • Other (please describe):

Changes in this PR

Describe the new behavior from this PR, and why it's needed
Issue #3656

Alternatives considered

All details are in the issue attached with this PR

@haricane8133 haricane8133 changed the title Enable hosting app in any route, with easy config Feat: Enable hosting app in any route, with easy config Aug 8, 2023
@haricane8133
Copy link
Contributor Author

@peterlau, any updates here? Please let me know if there's something I can do further in this PR

@@ -1,5 +1,7 @@
import { format, formatDuration, intervalToDuration } from "date-fns";
import _ from "lodash";
import { isEmpty as _isEmpty } from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend using import _isEmpty from 'lodash/isEmpty'.
Btw the above line imported lodash, so you need to use _.isEmpty only, don't need to re-import.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. I have fixed this by removing my duplicate import and using _.isEmpty

basename = new URL(packageJson.homepage).pathname;
} catch(e) {}
return _isEmpty(basename) ? '/' : basename;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing new line at end of file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a new line with the latest commit

@haricane8133
Copy link
Contributor Author

Fixed review comments and rebased my commits with upstream.


export function cleanDuplicateSlash(path) {
return path.replace(/([^:]\/)\/+/g, "$1");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is missing new line at the end of file too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@nhandt2021 nhandt2021 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me.

@haricane8133
Copy link
Contributor Author

@nhandt2021, @peterlau - could we get this PR merged if there are no further changes required?

@nhandt2021
Copy link
Contributor

@nhandt2021, @peterlau - could we get this PR merged if there are no further changes required?

I don't have permission to do that 😄

Copy link
Contributor

This PR is stale, because it has been open for 45 days with no activity. Remove the stale label or comment, or this will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 28, 2023
@haricane8133
Copy link
Contributor Author

@peterlau - could we get this PR merged if there are no further changes required?

@github-actions github-actions bot removed the Stale label Nov 29, 2023
@v1r3n v1r3n merged commit 0f187bf into Netflix:main Nov 29, 2023
@haricane8133
Copy link
Contributor Author

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants